-
Notifications
You must be signed in to change notification settings - Fork 294
Add Various Startup Types Support to the Price-taker Class #1713
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1713 +/- ##
==========================================
- Coverage 77.36% 73.67% -3.70%
==========================================
Files 394 397 +3
Lines 64980 65081 +101
Branches 10947 10962 +15
==========================================
- Hits 50274 47949 -2325
- Misses 12197 14629 +2432
+ Partials 2509 2503 -6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| CONFIG.declare( | ||
| "startup_types", | ||
| ConfigValue( | ||
| domain=dict, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggest using a more rigorous domain validation method.
def is_valid_startup_types(data):
if not isinstance(data, dict):
raise TypeError("Data must be a dict")
if len(data) == 0:
raise ConfigurationError("Received an empty dictionary for startup types")
for key, value in data.items():
if not isinstance(key, str):
raise TypeError("key must be a str")
if not isinstance(value, int):
raise TypeError("value must be an int")
# Values must be unique, as they correspond to different startup types
if len(data.values()) > len(set(data.values())):
raise ConfigurationError("Startup time for two or more startup types is the same.")
# Return a dictionary after sorting based on values
return dict(sorted(data.items(), key=lambda item: item[1]))You can use this domain validator as:
ConfigValue(domain=is_valid_startup_types, doc="...")This way, you don't have to check for empty dictionaries in your main code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have updated the codes based on your comments.
| CONFIG.declare( | ||
| "startup_types", | ||
| ConfigValue( | ||
| domain=dict, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can use the above domain validator: is_valid_startup_types
|
|
||
| if not startup_transition_time: | ||
| # if there is only one startup type, return | ||
| # startup_transition_time can be None or empty dict |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With the domain validator, empty dictionaries are no longer valid inputs. You just need to check that it is not None. For improved code readability, I suggest:
if startup_transition_time is None:
return| return | ||
|
|
||
| # multiple startup types | ||
| if startup_transition_time: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can remove this if condition and de-indent the code.
| minimum_down_time, startup_transition_time[startup_names[0]] | ||
| ) | ||
|
|
||
| # add a check to ensure the startup time is monotonically increasing. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Domain validator returns a sorted dictionary. So, this check can now be removed.
radhakrishnatg
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a few comments to improve code readability and maintainability. We can discuss the comments in our next meeting.
| def is_valid_startup_types(data): | ||
| """Validate if the startup_types received is valid""" | ||
|
|
||
| if data is None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This check is not needed. If the argument value is None, then Pyomo's ConfigDict would not call the domain validator.
radhakrishnatg
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code looks good, but I still have a few minor changes.
| ) | ||
|
|
||
| if self.config.startup_types: | ||
| # self.config.startup_types can be None or an empty dict |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor change: For better code readability, please change the condition to if self.config.startup_types is not None:. Also, please remove the comment, as it is no longer applicable (may create confusion for future maintainers).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
|
|
||
| # multiple startup types | ||
| if startup_transition_time: | ||
| # there will be at least two types of startup |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As mentioned earlier, the if condition check is now unnecessary. You can remove the check, and remove the additional indentation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
| if not isinstance(data, dict): | ||
| raise TypeError("Data must be a dictionary.") | ||
|
|
||
| if len(data) == 0: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on your tests, I'm wondering if we should change this to if len(data) <= 1:. This method must be used only when there is more than one startup type. If you have only startup type, then this feels unnecessary (Also, we will have to check the minimum uptime and startup time values to make sure they are consistent). Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I think the suggestion makes sense to me. Eq. 54 in Ben's paper once confused me until I asked for clarification from him. If {"hot": 4, "warm": 8, "cold": 12}, when the downtime is between 4 to 8, it is a hot start; 8 to 12, warm start and 12 to infinity, a cold start. This is important to understand eq54, if only one startup_transition_time is given, eq54 will break down.
I will add this example to the comments.
Fixes
This PR adds new features to the price-taker class.
Summary/Motivation:
The unit commitment model in the current price-taker class only supports a single type of startup. However, some generators have different startup types (e.g., hot and cold start) because their physical components heat up and cool down over time, so the time since last shutdown affects how much fuel it costs, and how much wear-and-tear is incurred to bring them back online.
The startup model is based on Knueven, Bernard, James Ostrowski, and Jean-Paul Watson. "On mixed-integer programming formulations for the unit commitment problem." INFORMS Journal on Computing 32, no. 4 (2020): 857-876.
Changes proposed in this PR:
Legal Acknowledgement
By contributing to this software project, I agree to the following terms and conditions for my contribution: